Enhancement 235: Enable ManifestWorkReplicaSet by default - #178
Conversation
Propose changing the MWRS feature gate default from false to true so it is enabled out of the box without requiring ClusterManager CR configuration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: august simonelli <august@redhat.com>
WalkthroughThis PR adds a formal enhancement proposal to enable the ChangesEnable ManifestWorkReplicaSet by Default
🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@enhancements/sig-architecture/235-enable-mwrs-by-default/README.md`:
- Around line 92-103: The prerequisites vs blocking criteria are inconsistent:
the "Prerequisites that must be resolved before this change lands" list includes
issue `#1198` but the "Blocking criteria" section (around lines 149-153) only
lists `#1522` and `#1380`; update the document so both sections match by either
moving `#1198` into the Blocking criteria or removing it from the Prerequisites
list, and ensure the headings "Prerequisites that must be resolved before this
change lands" and "Blocking criteria" mention the same set of issues (`#1522`,
`#1380`, and `#1198` if it is intended to block) so merge readiness is unambiguous.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 481df16d-14df-4d28-b467-8998e208f690
📒 Files selected for processing (2)
enhancements/sig-architecture/235-enable-mwrs-by-default/README.mdenhancements/sig-architecture/235-enable-mwrs-by-default/metadata.yaml
| **Prerequisites that must be resolved before this change lands:** | ||
|
|
||
| - [#1522](https://github.com/open-cluster-management-io/ocm/issues/1522): | ||
| MWRS reports `PlacementDecisionEmpty` when ManifestWork apply fails, masking | ||
| the real error. A default-enabled feature must not hide errors from users. | ||
| - [#1380](https://github.com/open-cluster-management-io/ocm/issues/1380): | ||
| Should use `generateName` to create ManifestWork from MWRS. | ||
| - [#1198](https://github.com/open-cluster-management-io/ocm/issues/1198): | ||
| Advanced MWRS use-cases need to be validated or documented as known | ||
| limitations. | ||
|
|
||
| NOTE: There may be more. See Open Questions below. |
There was a problem hiding this comment.
Align blocking criteria with stated prerequisites.
Line 92 says #1198 must be resolved before landing, but Lines 149-153 only list #1522 and #1380 as blocking criteria. Please make these sections consistent so merge readiness is unambiguous.
Suggested doc diff
- - All blocking bugs resolved (`#1522`, `#1380`).
+ - All blocking prerequisites resolved (`#1522`, `#1380`, `#1198`), or `#1198` explicitly documented as non-blocking with accepted limitations.Also applies to: 149-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@enhancements/sig-architecture/235-enable-mwrs-by-default/README.md` around
lines 92 - 103, The prerequisites vs blocking criteria are inconsistent: the
"Prerequisites that must be resolved before this change lands" list includes
issue `#1198` but the "Blocking criteria" section (around lines 149-153) only
lists `#1522` and `#1380`; update the document so both sections match by either
moving `#1198` into the Blocking criteria or removing it from the Prerequisites
list, and ensure the headings "Prerequisites that must be resolved before this
change lands" and "Blocking criteria" mention the same set of issues (`#1522`,
`#1380`, and `#1198` if it is intended to block) so merge readiness is unambiguous.
mikeshng
left a comment
There was a problem hiding this comment.
+1
MWRS is a feature that has been used by the community for a while now.
My opinion is that it might not be necessary to resolve all the prereq issues you have listed. They are not blocking issues as far as I can tell.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: augustrh, mikeshng The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@annelau21 @ncr38 please sign off or provide feedback if object. Thanks! |
|
|
||
| - 2024: MWRS introduced behind a feature gate (default disabled). | ||
| - Enhancement 229: Rollback support for MWRS (building on MWRS as foundation). | ||
| - Enhancement 231: Rollout plugin extensibility for MWRS. |
There was a problem hiding this comment.
229 and 231 has not been implemented yet, I think we need to setup a plan for these. Also we need a plan for promoting to beta.
There was a problem hiding this comment.
229 we are working on, we can share our work maybe in 2nd week of July
Summary
Propose changing the MWRS feature gate default from
falsetotrueso it is enabled out of the box without requiring explicit ClusterManager CR configuration.Motivation
Prerequisites
/cc @mikeshng
Summary by CodeRabbit